-
Notifications
You must be signed in to change notification settings - Fork 23.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure collection is a list if a str is given #69081
Conversation
collection_list = [collection_list] | ||
# We allow collections to be either a string or a list, so make sure | ||
# we have a list going forward. | ||
if isinstance(collection_list, string_types): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we insert an elif not isinstance(collection_list, (None, list)):
and fail? As it's written, someone could supply all sorts of other incorrect data types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. If I make it a dict, we get the same traceback about not being able to call append(). I'll make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of custom code, why just not force validation? this would handle 'non lists' already correctly and since 'collections' is static, we don't have to worry about templating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcoca If we assume CollectionSearch is always mixed in with Base (seems like it is?), then we can call self.get_validated_value() early on. I'm unclear on if the mixin is a safe assumption going forward though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i really want to redesign it so 'get_validated_value' is the normal 'getter' and that 'get_unvalidated' becomes the method for when we need 'direct' access.
@Shrews this PR contains the following merge commits: Please rebase your branch to remove these commits. |
Because we are doing work on modifying the collections value before it is actually validated, we can validate it ourselves early to make sure the user supplies either a string or list. Dicts are not valid. The new validation allows us to simplify the _ensure_default_collection() function. And since the field is now static, we no longer need to specify a default for it, which also allows us to simplify the function. Since the default is now removed, we can also remove the sanity/ignore.txt entry for collectionsearch.py. New unit tests are added (and the existing one modified) that allow us to make sure that we throw a parser error if a user specifies something other than a string or list for the collections value everywhere it can be specified.
The default is actually used, so restore it.
SUMMARY
Supplying the collection name as a string instead of a list would cause a traceback
when we attempted to append the 'ansible.legacy' collection to the list we expected.
Fixes #69054
ISSUE TYPE
COMPONENT NAME
collections
ADDITIONAL INFORMATION
test.yaml
results in: